Add Atom version to the printed version information #745

virtuoushub · 2017-09-13T19:00:41Z

Alternate Designs

Using spawn in getPythonVersion, similar to @arabelle's approach. See: https://github.com/arabelle/apm/blob/59f70d50571090fe205bf6b03c03f0c86398f5f4/src/apm-cli.coffee#L132-L143

I did not want to spawn a new process and parse it's output since I have access to Atom's package.json

Benefits

Closes #382

Possible Drawbacks

~~There is a workaround due to an issue on macOS I ran into while testing. Ideally, the workaround that checks with getResourcePath is not used.~~ Removed workaround see #745 (comment)

Applicable Issues

#453, #473 ~~, and atom/atom#6604~~

/cc @jebschiefer and @arabelle who worked on original PRs

virtuoushub · 2017-09-17T23:23:38Z

I rebased on top of 8fb7456 today. Please let me know if there is anything on my end that I am able to help with. [edit for grammar]

winstliu · 2017-09-25T21:50:58Z

src/apm-cli.coffee

+
+getAtomVersion = (callback) ->
+  config.getResourcePath (resourcePath) ->
+    # workaround if app.asar does not exist. see: https://github.com/atom/atom/issues/6604


is 6604 still a problem? There was only one report of it.

I ran into it while testing. For whatever reason app.asar does not exist on my machine.

~ ❯❯❯ sw_vers ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G1611

~/D/p/apm ❯❯❯ ls /Applications/Atom.app/Contents/Resources/ atom-version LICENSE.md bg.lproj de.lproj en_GB.lproj fi.lproj he.lproj it.lproj lv.lproj nl.lproj ru.lproj sw.lproj uk.lproj am.lproj bn.lproj el.lproj es.lproj fil.lproj hi.lproj ja.lproj ml.lproj pl.lproj sk.lproj ta.lproj vi.lproj app ca.lproj electron.asar es_419.lproj file.icns hr.lproj kn.lproj mr.lproj pt_BR.lproj sl.lproj te.lproj zh_CN.lproj ar.lproj cs.lproj electron.icns et.lproj fr.lproj hu.lproj ko.lproj ms.lproj pt_PT.lproj sr.lproj th.lproj zh_TW.lproj atom.icns da.lproj en.lproj fa.lproj gu.lproj id.lproj lt.lproj nb.lproj ro.lproj sv.lproj tr.lproj

~/D/p/apm ❯❯❯ apm --version atom-version apm 1.18.4 npm 3.10.10 node 6.9.5 x64 python 2.7.10 git 2.14.2

~/D/p/apm ❯❯❯ atom --version atom-version Atom : 1.20.1 Electron: 1.6.9 Chrome : 56.0.2924.87 Node : 7.4.0

winstliu · 2017-09-25T21:52:53Z

src/apm-cli.coffee

+    if not fs.existsSync(resourcePath)
+      resourcePath = resourcePath.replace('.asar', path.sep)
+    try
+      {version} = require(path.join(resourcePath, 'package.json')) ? {}


It would probably be better to return unknown rather than an empty object.

Based on @50Wliu's suggestion. See https://github.com/atom/apm/pull/745\#discussion_r140909097

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

virtuoushub · 2017-09-29T00:39:25Z

@50Wliu It will now return unknown instead of an empty object. ~~Please see 99bec86. Amended previously mentioned commit into the original. Please now see 4abb7ce. Fixed copy 🍝 in the previously mentioned commit. Please now see 53cccbc~~ I reread CONTRIBUTING.md and placed the requires in the proper order. Please see 90b79dd.

winstliu · 2017-10-16T11:57:54Z

spec/apm-cli-spec.coffee

+path = require 'path'
+temp = require 'temp'
 fs = require 'fs'
+


Remove empty newline

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

virtuoushub · 2017-10-17T11:42:50Z

@50Wliu I removed the empty newline.

#745 (comment)

winstliu · 2017-10-17T19:56:42Z

src/apm-cli.coffee

+            apm: apmVersion
+            npm: npmVersion
+            node: nodeVersion
+            atomVersion: atomVersion


Is there a reason this is atomVersion and not just atom?

Other than copy 🍝 , no. Fixed in ffdb043

winstliu · 2017-10-17T19:57:47Z

src/apm-cli.coffee

+getAtomVersion = (callback) ->
+  config.getResourcePath (resourcePath) ->
+    unknownVersion = 'unknown'
+    # workaround if app.asar does not exist. see: https://github.com/atom/atom/issues/6604


We haven't seen any reports of 6604 in a while. Is this needed? Did you run into it?

Yes. #745 (comment)

Whoops! Totally forgot I commented about it before 😊

It seems Atom 1.21 added back app.asar, see: #738 (comment)

For users who have Atom 1.20 and any other versions which do not use app.asar the workaround check seems to be needed.

Ok, good to know. Since apm is bundled with Atom and can't be installed manually, I think it's safe to remove this check since it'll be bundled with at least Atom 1.23 at this point.

Done. See e397021

See https://github.com/atom/apm/pull/738\#issuecomment-337573518

winstliu · 2017-10-19T08:27:32Z

spec/apm-cli-spec.coffee

+      testAtomVersion = '0.0.0'
+      tempAtomResourcePath = temp.mkdirSync('apm-resource-dir-')
+      fs.writeFileSync(path.join(tempAtomResourcePath, 'package.json'), JSON.stringify(version: testAtomVersion))
+      process.env.ATOM_RESOURCE_PATH = tempAtomResourcePath


This is reset at the end of the test, correct? If not it needs to be.

When you say this, do you mean process.env.ATOM_RESOURCE_PATH?

If so, I am basing this test logic off of other -spec examples. I do not see where it is reset. Is there an example of how to reset it?

apm/spec/list-spec.coffee

Lines 38 to 47 in 5cf83aa

resourcePath = temp.mkdirSync('apm-resource-path-')

atomPackages =

'test-module':

metadata:

name: 'test-module'

version: '1.0.0'

fs.writeFileSync(path.join(resourcePath, 'package.json'), JSON.stringify(_atomPackages: atomPackages))

process.env.ATOM_RESOURCE_PATH = resourcePath

atomHome = temp.mkdirSync('apm-home-dir-')

process.env.ATOM_HOME = atomHome

apm/spec/install-spec.coffee

Lines 24 to 25 in c9f0210

resourcePath = temp.mkdirSync('atom-resource-path-')

process.env.ATOM_RESOURCE_PATH = resourcePath

apm/spec/upgrade-spec.coffee

Lines 143 to 144 in d55f94a

it "logs an error when the installed location of Atom cannot be found", ->

process.env.ATOM_RESOURCE_PATH = '/tmp/atom/is/not/installed/here'

apm/spec/publish-spec.coffee

Lines 19 to 22 in d399d88

atomHome = temp.mkdirSync('apm-home-dir-')

process.env.ATOM_HOME = atomHome

process.env.ATOM_API_URL = "http://localhost:3000/api"

process.env.ATOM_RESOURCE_PATH = temp.mkdirSync('atom-resource-path-')

apm/spec/rebuild-spec.coffee

Lines 30 to 35 in b758628

atomHome = temp.mkdirSync('apm-home-dir-')

process.env.ATOM_HOME = atomHome

process.env.ATOM_ELECTRON_URL = "http://localhost:3000/node"

process.env.ATOM_PACKAGES_URL = "http://localhost:3000/packages"

process.env.ATOM_ELECTRON_VERSION = 'v0.10.3'

process.env.ATOM_RESOURCE_PATH = temp.mkdirSync('atom-resource-path-')

Ah ok, it is reset. You linked to it in the first code snippet.

apm/spec/list-spec.coffee

Line 45 in 5cf83aa

process.env.ATOM_RESOURCE_PATH = resourcePath

winstliu · 2017-10-24T08:15:06Z

👍

thomasjo changed the title ~~apm --version lists Atom version~~ Add Atom version to the printed version information Sep 14, 2017

virtuoushub force-pushed the atom-version branch from 2322ed5 to ea2fb36 Compare September 17, 2017 15:17

winstliu reviewed Sep 25, 2017

View reviewed changes

virtuoushub force-pushed the atom-version branch from ea2fb36 to 99bec86 Compare September 27, 2017 09:58

virtuoushub referenced this pull request in virtuoushub/apm Sep 27, 2017

Return unknown instead of an empty object

99bec86

Based on @50Wliu's suggestion. See https://github.com/atom/apm/pull/745\#discussion_r140909097

virtuoushub force-pushed the atom-version branch from 99bec86 to 4abb7ce Compare September 29, 2017 00:19

virtuoushub pushed a commit to virtuoushub/apm that referenced this pull request Sep 29, 2017

Add Atom version to the printed version information

4abb7ce

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

virtuoushub force-pushed the atom-version branch from 4abb7ce to 53cccbc Compare September 29, 2017 00:26

virtuoushub pushed a commit to virtuoushub/apm that referenced this pull request Sep 29, 2017

Add Atom version to the printed version information

53cccbc

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

virtuoushub force-pushed the atom-version branch from 53cccbc to 90b79dd Compare September 29, 2017 00:31

virtuoushub pushed a commit to virtuoushub/apm that referenced this pull request Sep 29, 2017

Add Atom version to the printed version information

90b79dd

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

winstliu reviewed Oct 16, 2017

View reviewed changes

spec/apm-cli-spec.coffee Outdated

path = require 'path'

temp = require 'temp'

fs = require 'fs'

Copy link

Contributor

winstliu Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty newline

Add Atom version to the printed version information

ad5cbde

Closes atom#382 See atom#453 and atom#473 Returning unknown instead of an empty object based on @50Wliu's suggestion. See atom#745

virtuoushub force-pushed the atom-version branch from 90b79dd to ad5cbde Compare October 17, 2017 11:40

winstliu reviewed Oct 17, 2017

View reviewed changes

Fix variable name to align with the rest

ffdb043

This was referenced Oct 18, 2017

ENOENT opening app.asar atom/atom#6604

Closed

Revert "Stop using asar archives" #738

Merged

Remove workaround as asar was added back in Atom 1.21

e397021

See https://github.com/atom/apm/pull/738\#issuecomment-337573518

winstliu reviewed Oct 19, 2017

View reviewed changes

winstliu merged commit ba72c63 into atom:master Oct 24, 2017

virtuoushub deleted the atom-version branch October 24, 2017 19:51

	resourcePath = temp.mkdirSync('apm-resource-path-')
	atomPackages =
	'test-module':
	metadata:
	name: 'test-module'
	version: '1.0.0'
	fs.writeFileSync(path.join(resourcePath, 'package.json'), JSON.stringify(_atomPackages: atomPackages))
	process.env.ATOM_RESOURCE_PATH = resourcePath
	atomHome = temp.mkdirSync('apm-home-dir-')
	process.env.ATOM_HOME = atomHome

	resourcePath = temp.mkdirSync('atom-resource-path-')
	process.env.ATOM_RESOURCE_PATH = resourcePath

	it "logs an error when the installed location of Atom cannot be found", ->
	process.env.ATOM_RESOURCE_PATH = '/tmp/atom/is/not/installed/here'

	atomHome = temp.mkdirSync('apm-home-dir-')
	process.env.ATOM_HOME = atomHome
	process.env.ATOM_API_URL = "http://localhost:3000/api"
	process.env.ATOM_RESOURCE_PATH = temp.mkdirSync('atom-resource-path-')

	atomHome = temp.mkdirSync('apm-home-dir-')
	process.env.ATOM_HOME = atomHome
	process.env.ATOM_ELECTRON_URL = "http://localhost:3000/node"
	process.env.ATOM_PACKAGES_URL = "http://localhost:3000/packages"
	process.env.ATOM_ELECTRON_VERSION = 'v0.10.3'
	process.env.ATOM_RESOURCE_PATH = temp.mkdirSync('atom-resource-path-')

Add Atom version to the printed version information #745

Add Atom version to the printed version information #745

Uh oh!

Conversation

virtuoushub commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Uh oh!

virtuoushub commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virtuoushub commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virtuoushub commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virtuoushub Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winstliu commented Oct 24, 2017

Uh oh!

Uh oh!

virtuoushub commented Sep 13, 2017 •

edited

Loading

virtuoushub commented Sep 17, 2017 •

edited

Loading

virtuoushub Oct 18, 2017 •

edited

Loading